Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix multiple installersets for tektonaddon #574

Merged
merged 1 commit into from
Jan 13, 2022
Merged

Fix multiple installersets for tektonaddon #574

merged 1 commit into from
Jan 13, 2022

Conversation

savitaashture
Copy link
Contributor

Changes

This PR fixes creation of multiple installer set for clustertasks addon

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

See the contribution guide for more details.

Release Notes

NONE

@tekton-robot tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jan 6, 2022
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 6, 2022
vdemeester
vdemeester reviewed Jan 6, 2022
View reviewed changes
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@savitaashture can we have some tests ? (not sure how easy it would be but still)

@savitaashture savitaashture force-pushed the fix-multiple-installerset-addons branch from eeeb699 to ee232ad Compare January 7, 2022 12:07
@savitaashture
Copy link
Contributor Author

@savitaashture can we have some tests ? (not sure how easy it would be but still)

Updated e2e test to verify installerSets for addon based on label

@savitaashture savitaashture force-pushed the fix-multiple-installerset-addons branch from ee232ad to 95c0925 Compare January 7, 2022 12:10
sm43
sm43 reviewed Jan 10, 2022
View reviewed changes
pkg/reconciler/kubernetes/tektoninstallerset/const.go Outdated
TriggersResourcesInstallerSet = "TriggersResources"
ConsoleCLIInstallerSet = "ConsoleCLI"
MiscellaneousResourcesInstallerSet = "MiscellaneousResources"
CreatedByValue = "TektonAddon"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are addon specific const right?
should we keep them in addon pkg? and not in tektoninstallerset?

sm43
sm43 reviewed Jan 10, 2022
View reviewed changes
pkg/reconciler/openshift/tektonaddon/tektonaddon.go Outdated
ctIs, err := oc.OperatorV1alpha1().TektonInstallerSets().
Get(ctx, compInstallerSet, metav1.GetOptions{})
// Delete installer sets if exist more than one
if len(installerSets.Items) > 1 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here if there are more than 1 installer set we are deleting all right?
then do we need to continue after deleting?

as after deleting
we are looping on items

for i := range installerSets.Items {

but we have already deleted all

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need a return statement I think after deleting

sm43
sm43 reviewed Jan 10, 2022
View reviewed changes
pkg/reconciler/openshift/tektonaddon/tektonaddon.go
@@ -369,26 +354,34 @@ func (r *Reconciler) ensureClusterTasks(ctx context.Context, ta *v1alpha1.Tekton
// and if installer set which already exist is of older version then it deletes and return false to create a new
// installer set
func checkIfInstallerSetExist(ctx context.Context, oc clientset.Interface, relVersion string,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as this is more generalised now, can we move this to installer Set pkg
so that other component can use the same?
something like installerSet.GetByLabel()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sm43 Shall i do that as separate PR once @pradeepitm12 changes are also in for this multiple installerset fix for Pipeline and Triggers
and together will do one refactoring PR
WDYT?
/cc @nikhil-thomas

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

separate pr sound fine.
could you create and issue (or jira) to track this.

@savitaashture savitaashture force-pushed the fix-multiple-installerset-addons branch from 95c0925 to 29222a5 Compare January 11, 2022 08:30
@savitaashture
Copy link
Contributor Author

@sm43 @nikhil-thomas I have addressed review comments except refactoring part
Planning to do refactor for checkIfInstallerSetExist createInstallerSet and makeInstallerSet common for all
as a separate PR once @pradeepitm12 changes are also in for this multiple installerset fix for Pipeline and Triggers

WDYT?

sm43
sm43 reviewed Jan 11, 2022
View reviewed changes
pkg/reconciler/openshift/tektonaddon/extension.go
@@ -134,7 +131,7 @@ func (oe openshiftExtension) PostReconcile(ctx context.Context, comp v1alpha1.Te
}

// spec hash stored on installerSet
lastAppliedHash := installedTIS.GetAnnotations()[tektoninstallerset.LastAppliedHashKey]
lastAppliedHash := installedTIS.Items[0].GetAnnotations()[tektoninstallerset.LastAppliedHashKey]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we have a check for len for Items ? just to be safe before accessing

@savitaashture savitaashture force-pushed the fix-multiple-installerset-addons branch from 29222a5 to ec659de Compare January 11, 2022 09:42
@nikhil-thomas
Copy link
Member

@savitaashture could you make 2 changes

  1. https://github.com/tektoncd/operator/pull/574/files#r782647681
  2. (minor comment which will help future readers 🧑‍💻 ) Fix multiple installersets for tektonaddon #574 (comment)

@savitaashture savitaashture force-pushed the fix-multiple-installerset-addons branch from ec659de to 7401a02 Compare January 12, 2022 08:40
@savitaashture
Copy link
Contributor Author

savitaashture commented Jan 12, 2022 •
edited
Loading

@savitaashture could you make 2 changes

  1. https://github.com/tektoncd/operator/pull/574/files#r782647681
  2. (minor comment which will help future readers technologist ) Fix multiple installersets for tektonaddon #574 (comment)

@nikhil-thomas Addressed Thank you 👍

@savitaashture savitaashture force-pushed the fix-multiple-installerset-addons branch from 7401a02 to 99edd78 Compare January 12, 2022 09:01
@nikhil-thomas
Copy link
Member

✅ lgtm

@nikhil-thomas
Copy link
Member

/approve

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nikhil-thomas

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 12, 2022
@sm43
Copy link
Member

sm43 commented Jan 13, 2022

LGTM

@sm43 sm43 added the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2022
@tekton-robot tekton-robot merged commit 22b2ed5 into tektoncd:main Jan 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vdemeester vdemeester vdemeester left review comments

@nikhil-thomas nikhil-thomas nikhil-thomas left review comments

@pradeepitm12 pradeepitm12 pradeepitm12 left review comments

@sm43 sm43 sm43 left review comments

@concaf concaf Awaiting requested review from concaf

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@savitaashture @nikhil-thomas @tekton-robot @sm43 @vdemeester @pradeepitm12